Repository navigation
mcp: accept Windows drive letter file roots - #1443
akshita317 wants to merge 3 commits into
Conversation
fileRoot turns the client's roots into absolute file paths for FileResourceHandler. On Windows a root for a drive letter path is a URI such as file:///C:/Users/me/project (RFC 8089, Appendix E.2), whose path is /C:/Users/me/project. fileRoot converted that to \C:\Users\me\project, which is not an absolute path on Windows, so every such root failed with "not an absolute path", and with it every read through FileResourceHandler once the client had any roots. Drop the slash before a drive letter on Windows. VS Code's form, file:///c%3A/..., decodes to the same path. TestFileRoot is no longer skipped on Windows and gets drive letter cases there; they fail without the change. TestFileURIPath covers the conversion for both Windows and other systems on any platform.
chrikrah
left a comment
chrikrah
left a comment
There was a problem hiding this comment.
From my perspective this can be merged. Passing goos in is what makes it hold: no workflow here runs on Windows, so the new TestFileRoot cases never execute in CI, while TestFileURIPath does on Linux. This takes up the Windows work 8361df7 deferred for roots only; computeURIFilepath still skips there.
$ go test ./mcp/ -run 'TestFileRoot|TestFileURIPath' -count=1 # head c9e6d6b, merge base 6f47cb7, go1.27.1 linux/amd64
ok github.com/modelcontextprotocol/go-sdk/mcp 0.011s
$ <prefix the drive-letter condition in fileURIPath with false> && <same command>
--- FAIL: TestFileURIPath (0.00s)
resource_test.go:75: fileURIPath("/C:/foo", "windows") = "/C:/foo", want "C:/foo"
FAIL
$ GOOS=windows go test -c -o /dev/null ./mcp/ && go test ./... -count=1
# compiles; 14 packages ok
# not run: the Windows TestFileRoot cases, no Windows host here
Did you run those cases on a Windows machine?
| @@ -186,13 +187,28 @@ func fileRoot(root *Root) (_ string, err error) { | |||
| return "", errors.New("empty path") | |||
| } | |||
| // We don't want Localize here: we want an absolute path, which is not local. | |||
There was a problem hiding this comment.
Nit: while we're here, let's drop this comment:
|
|
||
| func TestFileRoot(t *testing.T) { | ||
| if runtime.GOOS == "windows" { | ||
| t.Skip("TODO: fix for Windows") |
There was a problem hiding this comment.
There are 3 other TODO: fix for Windows skips in this file and 1 more in mcp/mcp_test.go. Since roots now work on Windows, could we address and unskip those too?
There was a problem hiding this comment.
Done in 16a042b. All three skips in resource_test.go are gone, and so is the TODO: fix for Windows block in TestEndToEnd. That one was failing on Windows because the test itself built the root as "file://" + rootAbs, which gives file://C:\..., not a file URI. It now builds the URI with net/url. Most of the mcp_test.go diff is re-indentation from removing the if block; it reads better with whitespace hidden.
| if goos == "windows" && len(uriPath) >= 3 && uriPath[0] == '/' && uriPath[2] == ':' && isASCIILetter(uriPath[1]) { | ||
| return uriPath[1:] | ||
| } | ||
| return uriPath |
There was a problem hiding this comment.
let's handle UNC path as well
There was a problem hiding this comment.
Done in 16a042b. On Windows, a host other than localhost is now a UNC path (RFC 8089, Appendix E.3.1): file://server/share/foo gives \server\share\foo, and file://localhost/C:/foo still gives C:\foo. On other systems the host is ignored as before. fileURIPath takes the host now, and TestFileURIPath and TestFileRoot have cases for both.
A file URI with a host other than localhost names a UNC path on Windows (RFC 8089, Appendix E.3.1), so file://server/share/dir is now the root \server\share\dir rather than a path that drops the host. With file roots working on Windows, the resource tests skipped there pass, so remove the skips. TestEndToEnd built its root as "file://" followed by the path, which on Windows is not a file URI (file://C:\dir); build it with net/url instead. Drop the stale comment about Localize in fileRoot.
|
@chrikrah Yes. Here they are on a Windows 11 machine, at 16a042b (which also takes the follow-ups nahapetyan-serob asked for: UNC roots, and unskipping the other Windows tests): As you say, no CI workflow runs on Windows, so these only run on a machine like this one. |
fileRootturns the client's roots into absolute file paths forFileResourceHandler. On Windows, a root for a drive letter path is a URI such asfile:///C:/Users/me/project(RFC 8089, Appendix E.2), whose path is/C:/Users/me/project.fileRootconverted that to\C:\Users\me\project, which is not an absolute path on Windows, so every such root failed withnot an absolute path, and with it every read throughFileResourceHandleras soon as the client reported any roots.TestFileRootwas skipped on Windows with a "TODO: fix for Windows".This drops the slash before a drive letter on Windows (
fileURIPath). VS Code's form,file:///c%3A/..., decodes to the same path. Nothing changes on other systems.Tests:
TestFileRootis no longer skipped on Windows and has drive letter cases there (upper and lower case,..,file:/C:/…,c%3A, andfile:///foostill rejected). They fail without the change; the existing cases run unchanged elsewhere.TestFileURIPathcovers the conversion forwindowsandlinuxon any platform, so CI on Linux checks it too.The other Windows-skipped tests in
resource_test.go(TestComputeURIFilepath,TestReadFileResource, …) use POSIX paths in their expectations; I left them for a separate change.AI assistance: prepared with Claude Code.